Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-19142] Prompt user whether to add the job to the current view #2529

Merged
merged 2 commits into from Sep 9, 2016

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Aug 31, 2016

https://issues.jenkins-ci.org/browse/JENKINS-19142

@jenkinsci/code-reviewers

This adds a checkbox in the new job screen, allowing the user to control whether the job will be added to the current view.

jenkins-37857

@daniel-beck daniel-beck changed the title [JENKINS-37857] Prompt user whether to add the job to the current view [JENKINS-19142] Prompt user whether to add the job to the current view Aug 31, 2016
@jtnord
Copy link
Member

jtnord commented Aug 31, 2016

what if the current view is based on a regex filter?

@Vlatombe
Copy link
Member Author

Vlatombe commented Aug 31, 2016

If a regex is set then the checkbox will be unticked by default (see isAddToCurrentView method), unless if there are also jobs added through jobNames.

@daniel-beck
Copy link
Member

PR build on Firefox 48:

screen shot 2016-09-01 at 10 03 47

@daniel-beck
Copy link
Member

Naming of the file is inconsistent with the vast majority of existing Jelly files.

~1/6 have dashes, about half of them to separate a prefix (config-) from the rest. Almost all others are lowercase or camelCase. Underscores are only used as prefix (e.g. _api.jelly).

I therefore recommend something like newJobButtonBar or newJob-buttonBar.

@Vlatombe
Copy link
Member Author

Vlatombe commented Sep 1, 2016

@daniel-beck Both fixed

@Vlatombe Vlatombe added the needs-more-reviews Complex change, which would benefit from more eyes label Sep 2, 2016
@batmat
Copy link
Member

batmat commented Sep 4, 2016

👍

I'm not sure I find the rule of checking/unchecking by default is definitely the right one. But as I can't come up with a better proposal, I'm fine with it.

If someone finds a better way, it'll be quite easy to adjust as Vincent isolated that well in a small decision method.

Also, used again docker run -ti -p 8080:8080 -e ID=2529 batmat/jenkins-pr-tester and happy to see it still works.

@daniel-beck
Copy link
Member

I think the defaults here are sensible. It's definitely a major improvement over the current situation.

Since the UI glitch has been resolved 👍

@daniel-beck daniel-beck added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Sep 4, 2016
@oleg-nenashev
Copy link
Member

👍

@oleg-nenashev oleg-nenashev merged commit f4c9eb0 into jenkinsci:master Sep 9, 2016
@Vlatombe Vlatombe deleted the JENKINS-37857 branch September 9, 2016 13:40
daniel-beck added a commit that referenced this pull request Sep 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
5 participants